Skip to content

make shared_helpers exe function work for both cygwin and non-cygwin hosts #141374

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 23, 2025

Conversation

jeremyd2019
Copy link
Contributor

On Cygwin, it needs to not append .exe, because /proc/self/exe (and therefore std::env::current_exe) does not include the .exe extension, breaking bootstrap's rustc wrapper. On hosts other than Cygwin, it does need to append .exe because the file really does have a .exe extension, and non-Cygwin hosts won't be doing the same filename rewriting that Cygwin does when looking for a file X but finding only X.exe in its place.

Arising from discussion in #140154 (review)
@mati865 @Berrysoft

@rustbot
Copy link
Collaborator

rustbot commented May 22, 2025

r? @onur-ozkan

rustbot has assigned @onur-ozkan.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels May 22, 2025
@rust-log-analyzer

This comment has been minimized.

@onur-ozkan
Copy link
Member

Thank you!

Could you please squash your commits into single commit? I am also not familiar with the Windows environment, so passing this to person that I kind a know that they are using Windows.

r? @jieyouxu

@rustbot rustbot assigned jieyouxu and unassigned onur-ozkan May 22, 2025
@jeremyd2019
Copy link
Contributor Author

sure, that was the point of the fixup! commit. I had a minor mishap pasting the change into github's web editor, and had an extra space in there.

Copy link
Contributor

@mati865 mati865 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about leaving a comment there? I feel like this condition will not be obvious, even to people familiar with Cygwin.

@jieyouxu
Copy link
Member

Yeah, please leave a comment. When I reviewed the original PR the change looked correct.

@jieyouxu
Copy link
Member

@rustbot author

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 22, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 22, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 22, 2025
…hosts

On Cygwin, it needs to not append .exe, because /proc/self/exe (and
therefore std::env::current_exe) does not include the .exe extension,
breaking bootstrap's rustc wrapper.  On hosts other than Cygwin, it
*does* need to append .exe because the file really does have a .exe
extension, and non-Cygwin hosts won't be doing the same filename
rewriting that Cygwin does when looking for a file X but finding only
X.exe in its place.
@jeremyd2019
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 22, 2025
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@jieyouxu
Copy link
Member

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented May 23, 2025

📌 Commit 1f862a8 has been approved by jieyouxu

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented May 23, 2025

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 23, 2025
@jieyouxu jieyouxu added the O-cygwin Target: *-pc-cygwin label May 23, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 23, 2025
make shared_helpers exe function work for both cygwin and non-cygwin hosts

On Cygwin, it needs to not append .exe, because /proc/self/exe (and therefore `std::env::current_exe`) does not include the .exe extension, breaking bootstrap's rustc wrapper.  On hosts other than Cygwin, it *does* need to append .exe because the file really does have a .exe extension, and non-Cygwin hosts won't be doing the same filename rewriting that Cygwin does when looking for a file X but finding only X.exe in its place.

Arising from discussion in rust-lang#140154 (review)
`@mati865` `@Berrysoft`
bors added a commit that referenced this pull request May 23, 2025
Rollup of 7 pull requests

Successful merges:

 - #138896 (std: fix aliasing bug in UNIX process implementation)
 - #140832 (aarch64-linux: Default to FramePointer::NonLeaf)
 - #141065 (Updated std doctests for wasm)
 - #141369 (Simplify `format_integer_with_underscore_sep`)
 - #141374 (make shared_helpers exe function work for both cygwin and non-cygwin hosts)
 - #141398 (chore: fix typos in comment)
 - #141457 (Update mdbook to 0.4.50)

Failed merges:

 - #141405 (GetUserProfileDirectoryW is now documented to always store the size)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8b69895 into rust-lang:master May 23, 2025
6 checks passed
@rustbot rustbot added this to the 1.89.0 milestone May 23, 2025
rust-timer added a commit that referenced this pull request May 23, 2025
Rollup merge of #141374 - jeremyd2019:patch-1, r=jieyouxu

make shared_helpers exe function work for both cygwin and non-cygwin hosts

On Cygwin, it needs to not append .exe, because /proc/self/exe (and therefore `std::env::current_exe`) does not include the .exe extension, breaking bootstrap's rustc wrapper.  On hosts other than Cygwin, it *does* need to append .exe because the file really does have a .exe extension, and non-Cygwin hosts won't be doing the same filename rewriting that Cygwin does when looking for a file X but finding only X.exe in its place.

Arising from discussion in #140154 (review)
``@mati865`` ``@Berrysoft``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-cygwin Target: *-pc-cygwin S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants